Move pipe and socket URL handling to net/url by samuv · Pull Request #5215 · stacklok/toolhive

samuv · 2026-05-07T13:09:12Z

Summary

Follow-up to #5201 (review thread). Stacked on socker-windows; will rebase onto main once #5201 merges. Sibling to #5214.

Replaces prefix-and-trim string handling with net/url.Parse on both producer (socketURL) and consumer (HTTPClientForURL, ParseNamedPipeURL, ParseUnixSocketPath) sides so pipe and socket addresses round-trip through one library and one set of rules.

The change includes one real bug fix: socketURL previously emitted unix://C:\path\thv.sock for Windows AF_UNIX paths, which url.Parse rejects with invalid port ":\\path\\thv.sock". The dispatcher only worked because it used strings.TrimPrefix. New behavior emits unix:///C:%5Cpath%5Cthv.sock, which round-trips cleanly. Round-trip is now exercised by TestSocketURL_RoundTrip_Unix and TestSocketURL_RoundTrip_NamedPipe.

Other improvements that fall out of the migration:

Case-insensitive scheme dispatch — NPIPE://x routes the same arm as npipe://x because url.Parse lowercases the scheme during parsing. Pinned by TestHTTPClientForURL_SchemeDispatchCaseInsensitive.
Case-insensitive pipe-name normalization — npipe://Thv-API resolves to \\.\pipe\thv-api via strings.ToLower(u.Hostname()); matches the kernel's case-insensitive pipe namespace.
Length cap on pipe names (247 chars, the CreateNamedPipeW lpName limit after the \\.\pipe\ prefix).
Reserved-name rejection for legacy Windows device names (CON, NUL, PRN, AUX, COM1-9, LPT1-9).
Positive charset of [A-Za-z0-9._-]+ for pipe names — replaces the over-eager strings.Contains(name, "..") guard which rejected legitimate names like my..api.
Stricter URL shape: ParseNamedPipeURL rejects pipe URLs with non-empty path / query / fragment / userinfo / port; ParseUnixSocketPath rejects unix URLs with non-empty authority.

Addresses inline review comments 3201085366 (ParseNamedPipeURL migration), 3201085375 (dispatcher migration), and 3201085383 (socketURL / ParseUnixSocketPath migration + Windows AF_UNIX bug).

Type of change

Bug fix (Windows AF_UNIX URL round-trip)
Code quality / hardening

Test plan

Unit tests (go test ./pkg/api/... ./pkg/server/discovery/...) — green on macOS; new round-trip tests, scheme-case test, reserved-name tests, length-cap test, and shape-rejection cases all pass.
task lint-fix — 0 issues.
GOOS=windows go vet and GOOS=windows go test -c -o /dev/null for both packages — both clean.

Does this introduce a user-facing change?

Yes (Windows-only). On Windows, socketURL for AF_UNIX paths now emits a percent-encoded URL form. The discovery file is local per-user and overwritten on every thv serve startup, so there is no on-disk migration concern. POSIX paths like /tmp/thv.sock produce identical output before and after.

Made with Cursor

codecov · 2026-05-07T13:16:07Z

Codecov Report

❌ Patch coverage is 77.14286% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 67.82%. Comparing base (8d9eb28) to head (b0ffadf).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/server/discovery/health.go	76.47%	4 Missing and 4 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5215      +/-   ##
==========================================
- Coverage   67.87%   67.82%   -0.05%     
==========================================
  Files         610      610              
  Lines       62383    62405      +22     
==========================================
- Hits        42340    42326      -14     
- Misses      16871    16903      +32     
- Partials     3172     3176       +4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

jhrozek

Two related issues on the Windows side. The af_unix branch of socketURL doesn't actually produce the URL the test fixture asserts, and there's no round-trip test for that branch — which is why the mismatch slipped through. Both should be addressable with a one-char producer change plus the matching round-trip test. Verified locally with Go 1.26.

Rest of the change reads fine.

socketURL on Windows for AF_UNIX paths emitted unix://C:%5Cpath%5Cthv.sock (two slashes) because url.URL.String() only inserts the host/path slash when Host != "". url.Parse then mis-reads the drive letter as host:port and returns "invalid port :%5Cpath%5Cthv.sock", breaking the producer/consumer contract that the rest of this PR establishes. Force the leading slash on Path so String() emits the three-slash form (unix:///C:%5Cpath%5Cthv.sock), which url.Parse accepts and which ParseUnixSocketPath already handles by stripping the synthetic slash before the drive letter. Add TestSocketURL_RoundTrip_AFUnix as a parallel to the existing named-pipe round-trip so future regressions on the Windows AF_UNIX side fail closed in CI rather than only being caught by reading the test fixture row. Addresses review on #5215. Co-authored-by: Cursor <cursoragent@cursor.com>

Replace prefix-and-trim string handling with net/url.Parse on both the producer (socketURL) and the consumer (HTTPClientForURL, ParseNamedPipeURL, ParseUnixSocketPath) sides so pipe and socket addresses round-trip through one library and one set of rules. socketURL now emits unix:///<path> via (&url.URL{}).String(), which fixes a real bug on Windows AF_UNIX paths: the previous concatenation form produced unix://C:\path\thv.sock, which url.Parse rejects with "invalid port :\\path\\thv.sock". The round-trip is exercised by new tests on both platforms. HTTPClientForURL now dispatches on url.Parse + switch u.Scheme, which gives case-insensitive scheme matching (NPIPE://x routes the same as npipe://x) for free and matches what the http arm already does via ValidateLoopbackURL. ParseNamedPipeURL gains explicit checks that net/url alone cannot do: a 247-character cap (CreateNamedPipeW's lpName limit minus the \\.\pipe\ prefix), the legacy reserved Windows device names (CON, NUL, COM1-9, LPT1-9, ...), and a positive charset of [A-Za-z0-9._-]+. Names are lowercased via strings.ToLower because the pipe namespace is case-insensitive at the kernel layer; the old strings.Contains(name, "..") guard is removed because it rejected legitimate names like my..api. ParseUnixSocketPath rejects URLs with non-empty authority, query, fragment, or userinfo; it also strips the synthetic leading slash that url.URL inserts in front of Windows drive letters so unix:///C:%5Cpath%5Cthv.sock round-trips back to C:\path\thv.sock. Co-authored-by: Cursor <cursoragent@cursor.com>

socketURL on Windows for AF_UNIX paths emitted unix://C:%5Cpath%5Cthv.sock (two slashes) because url.URL.String() only inserts the host/path slash when Host != "". url.Parse then mis-reads the drive letter as host:port and returns "invalid port :%5Cpath%5Cthv.sock", breaking the producer/consumer contract that the rest of this PR establishes. Force the leading slash on Path so String() emits the three-slash form (unix:///C:%5Cpath%5Cthv.sock), which url.Parse accepts and which ParseUnixSocketPath already handles by stripping the synthetic slash before the drive letter. Add TestSocketURL_RoundTrip_AFUnix as a parallel to the existing named-pipe round-trip so future regressions on the Windows AF_UNIX side fail closed in CI rather than only being caught by reading the test fixture row. Addresses review on #5215. Co-authored-by: Cursor <cursoragent@cursor.com>

aponcedeleonch

Approving since Jakub's feedback is also taken care of

jhrozek

Both Windows af_unix findings addressed cleanly in b0ffadf. The producer now emits the three-slash form, the new round-trip test closes the loop, and the expanded doc comment explains why the leading slash is load-bearing. LGTM.

samuv requested review from JAORMX and amirejaz as code owners May 7, 2026 13:09

samuv self-assigned this May 7, 2026

github-actions Bot added the size/S Small PR: 100-299 lines changed label May 7, 2026

This was referenced May 7, 2026

Pin npipe round-trip and pipe lifecycle invariants #5216

Merged

Add Windows named-pipe support to the API listener #5201

Merged

Base automatically changed from socker-windows to main May 7, 2026 13:42

samuv requested review from ChrisJBurns, jhrozek, lujunsan, rdimitrov and yrobla as code owners May 7, 2026 13:42

samuv force-pushed the windows-pipe-net-url branch from 9a83a9d to 24b1dfe Compare May 7, 2026 13:54

samuv requested a review from aponcedeleonch May 7, 2026 13:55

github-actions Bot added size/S Small PR: 100-299 lines changed and removed size/S Small PR: 100-299 lines changed labels May 7, 2026

jhrozek reviewed May 7, 2026

View reviewed changes

Comment thread pkg/api/socket_windows.go Outdated

Comment thread pkg/api/socket_windows_test.go

github-actions Bot added size/S Small PR: 100-299 lines changed and removed size/S Small PR: 100-299 lines changed labels May 8, 2026

samuv and others added 2 commits May 8, 2026 10:13

samuv force-pushed the windows-pipe-net-url branch from df44c25 to b0ffadf Compare May 8, 2026 08:13

github-actions Bot added size/S Small PR: 100-299 lines changed and removed size/S Small PR: 100-299 lines changed labels May 8, 2026

samuv requested a review from jhrozek May 8, 2026 09:17

aponcedeleonch approved these changes May 8, 2026

View reviewed changes

samuv merged commit a4d66b1 into main May 8, 2026
44 checks passed

samuv deleted the windows-pipe-net-url branch May 8, 2026 11:04

jhrozek approved these changes May 8, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move pipe and socket URL handling to net/url#5215

Move pipe and socket URL handling to net/url#5215
samuv merged 2 commits intomainfrom
windows-pipe-net-url

samuv commented May 7, 2026

Uh oh!

codecov Bot commented May 7, 2026 •

edited

Loading

Uh oh!

jhrozek left a comment

Uh oh!

Uh oh!

Uh oh!

aponcedeleonch left a comment

Uh oh!

Uh oh!

jhrozek left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

samuv commented May 7, 2026

Summary

Type of change

Test plan

Does this introduce a user-facing change?

Uh oh!

codecov Bot commented May 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jhrozek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

aponcedeleonch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jhrozek left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov Bot commented May 7, 2026 •

edited

Loading